fix: getGraphMetrics by using Float64 for multipliers in clickhouse queries#2411
Conversation
WalkthroughUpdated division multiplier types from UInt32 to Float64 across two analytics repository SQL queries to increase numeric precision during value calculations in metrics retrieval operations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts (1)
88-88: LGTM! Correctly fixes integer division truncation.The change from
UInt32toFloat64ensures floating-point division for accurate metric calculations. With integer division,100 / 30would truncate to3, but with Float64 it correctly computes3.3333..., which is then properly rounded to 4 decimal places.Consider applying the same fix to
granuledivisions:Similar division operations using
{granule:UInt32}exist elsewhere in this file (e.g., lines 157, 578-579) and may suffer from the same integer truncation issue. For consistency and precision, consider updating those as well:round(sum(TotalRequests) / {granule:Float64}, 4)Also applies to: 122-122
controlplane/src/core/repositories/analytics/SubgraphMetricsRepository.ts (1)
100-100: LGTM! Correctly mirrors the MetricsRepository fix.The change from
UInt32toFloat64ensures accurate floating-point division for subgraph metrics, consistent with the fix applied to MetricsRepository.ts.Consider applying the same fix to
granuledivisions:Similar division operations using
{granule:UInt32}exist elsewhere in this file (e.g., lines 169, 590-591) and may exhibit the same integer truncation behavior. For consistency and precision, consider updating those as well:round(sum(TotalRequests) / {granule:Float64}, 4)Also applies to: 134-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controlplane/src/core/repositories/analytics/MetricsRepository.ts(2 hunks)controlplane/src/core/repositories/analytics/SubgraphMetricsRepository.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2411 +/- ##
==========================================
+ Coverage 1.49% 62.31% +60.82%
==========================================
Files 292 295 +3
Lines 46926 41134 -5792
Branches 431 4194 +3763
==========================================
+ Hits 703 25634 +24931
+ Misses 45940 15480 -30460
+ Partials 283 20 -263 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ueries (wundergraph#2411) chore(release): Publish [skip ci] - wgc@0.102.5 - @wundergraph/composition@0.49.0 - controlplane@0.181.0 - graphqlmetrics@0.37.0 - @wundergraph/protographic@0.14.0 - router@0.273.0 - @wundergraph/cosmo-shared@0.42.24 - studio@0.147.0 feat: upgrade NextJS (wundergraph#2410) chore(release): Publish [skip ci] - studio@0.148.0 fix: resolve js-yaml vulnerabilities (wundergraph#2415) feat: validate session cookie (wundergraph#2406) Co-authored-by: Milinda Dias <83293842+SkArchon@users.noreply.github.com> chore(release): Publish [skip ci] - wgc@0.102.6 - controlplane@0.182.0 - keycloak@0.11.1 fix: codecov carryforward paths are not detected (wundergraph#2412) chore: bumping expr dependency (wundergraph#2419) fix: make error accessible to custom modules (wundergraph#2420) chore(release): Publish [skip ci] - router@0.273.1 fix: use the correct cgroup policy to work on most of the systems (wundergraph#2421) chore(release): Publish [skip ci] - router@0.273.2 Add support for per message deflate to websocket connectinos Merge branch 'main' into per-message-deflate revert formatting
…ueries (wundergraph#2411) chore(release): Publish [skip ci] - wgc@0.102.5 - @wundergraph/composition@0.49.0 - controlplane@0.181.0 - graphqlmetrics@0.37.0 - @wundergraph/protographic@0.14.0 - router@0.273.0 - @wundergraph/cosmo-shared@0.42.24 - studio@0.147.0 feat: upgrade NextJS (wundergraph#2410) chore(release): Publish [skip ci] - studio@0.148.0 fix: resolve js-yaml vulnerabilities (wundergraph#2415) feat: validate session cookie (wundergraph#2406) Co-authored-by: Milinda Dias <83293842+SkArchon@users.noreply.github.com> chore(release): Publish [skip ci] - wgc@0.102.6 - controlplane@0.182.0 - keycloak@0.11.1 fix: codecov carryforward paths are not detected (wundergraph#2412) chore: bumping expr dependency (wundergraph#2419) fix: make error accessible to custom modules (wundergraph#2420) chore(release): Publish [skip ci] - router@0.273.1 fix: use the correct cgroup policy to work on most of the systems (wundergraph#2421) chore(release): Publish [skip ci] - router@0.273.2 Add support for per message deflate to websocket connectinos Merge branch 'main' into per-message-deflate revert formatting
Summary by CodeRabbit
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist